Skip to content

Conversation

MathdudeMan
Copy link

@MathdudeMan MathdudeMan commented Jan 25, 2025

Overview: What does this pull request change?

Resolved 8 type errors in functions.py. Required adding a relevant type addition in scale.py.

Related issue: #3375

Reviewer Checklist

  • The PR title is descriptive enough for the changelog, and the PR is labeled correctly
  • If applicable: newly added non-private functions and classes have a docstring including a short summary and a PARAMETERS section
  • If applicable: newly added functions and classes are tested

@henrikmidtiby henrikmidtiby mentioned this pull request Aug 13, 2025
22 tasks
@henrikmidtiby
Copy link
Contributor

@MathdudeMan thanks for your contribution.

I can see that you have changed the type of the input function in the constructor for the ParametricFunction class, so it will be function: Callable[[float | np.ndarray], Point3DLike].

Can you describe why the np.ndarray part was added to the type annotation?
On my computer it seems to work well with only the float part of the type annotation.

@MathdudeMan
Copy link
Author

MathdudeMan commented Aug 14, 2025

On line 168, that same self.function is called with a Numpy array rather than a float, thus the need for an extra type.

@henrikmidtiby
Copy link
Contributor

@MathdudeMan
Good catch, I missed that the class ParametricFunction also supported vectorized functions.
In that case I think that you need to do something like

def internal_parametric_function(
    t: float | np.ndarray,
) -> Point3D | Point3D_Array:

to specify that the output of the internal_parametric_function could be an array.

@chopan050 What is your take on this? To me it seems like MathdudeMan has found a potential typing issue that is not detected by mypy. I can follow his logic but I don't understand why mypy think it is ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants